Skip to content

New rpc service to attach/detach a controller to a namespace.#237

Open
VaibhavJMarvell wants to merge 1 commit into
opiproject:mainfrom
VaibhavJMarvell:main
Open

New rpc service to attach/detach a controller to a namespace.#237
VaibhavJMarvell wants to merge 1 commit into
opiproject:mainfrom
VaibhavJMarvell:main

Conversation

@VaibhavJMarvell

Copy link
Copy Markdown

Added attach/detach controller to cater to use-case of an existing customer.

Signed-off-by: Vaibhav Jain vaibhavj@marvell.com
Signed-off-by: Michal Kalderon mkalderon@marvell.com

@VaibhavJMarvell VaibhavJMarvell requested a review from a team as a code owner January 25, 2023 18:18
@glimchb glimchb requested a review from a team January 25, 2023 18:43
@glimchb glimchb added the Storage APIs or code related to storage area label Jan 25, 2023

@glimchb glimchb left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how this si different from CreateNVMeNamespace ?

@VaibhavJMarvell

VaibhavJMarvell commented Jan 25, 2023

Copy link
Copy Markdown
Author

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of CreateNVMeNamespace, all the controllers are attached to the newly created namespace.

@glimchb

glimchb commented Jan 25, 2023

Copy link
Copy Markdown
Member

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of 'CreateNVMeNamespace', all the controllers are attached to the newly created namespace.

so maybe just add field ControllerId to existing CreateNVMeNamespace ? instead of new RPCs? see ca2c25b

@jainvipin jainvipin left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 on the need for this API i.e. the need to explicitly enable/disable the namespace within a controller.
a few minor comments inline @VaibhavJMarvell

(google.api.field_behavior) = REQUIRED
];
string controller = 2 [(google.api.field_behavior) = REQUIRED];
string nv_me_namespace_id = 3 [(google.api.field_behavior) = REQUIRED];

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should this be common.v1.ObjectKey which bytes all ids are like that. Same for controller?

@jainvipin

jainvipin commented Jan 26, 2023

Copy link
Copy Markdown
Contributor

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of 'CreateNVMeNamespace', all the controllers are attached to the newly created namespace.

so maybe just add field ControllerId to existing CreateNVMeNamespace ? instead of new RPCs? see ca2c25b

we could do that to, but this API would then become explicit enable/disable API to toggle namespace on controller.

Added attach/detach controller to cater to use-case of an existing customer.

Signed-off-by: Vaibhav Jain <vaibhavj@marvell.com>
Signed-off-by: Michal Kalderon <mkalderon@marvell.com>

@VaibhavJMarvell VaibhavJMarvell left a comment

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have changed the controller and namespace types to ObjectKey.

@glimchb

glimchb commented Jan 26, 2023

Copy link
Copy Markdown
Member

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of 'CreateNVMeNamespace', all the controllers are attached to the newly created namespace.

so maybe just add field ControllerId to existing CreateNVMeNamespace ? instead of new RPCs? see ca2c25b

we could do that to, but this API would then become explicit enable/disable API to toggle namespace on controller.

seems to me like we should use UpdateNVMeNamespace if we want to toggle enable/disable bit... instead of new RPC

@jainvipin

Copy link
Copy Markdown
Contributor

how this si different from CreateNVMeNamespace ?

We have an existing customer whose use-case requires attaching/detaching specific controllers to a namespace. In the current scenario of 'CreateNVMeNamespace', all the controllers are attached to the newly created namespace.

so maybe just add field ControllerId to existing CreateNVMeNamespace ? instead of new RPCs? see ca2c25b

we could do that to, but this API would then become explicit enable/disable API to toggle namespace on controller.

seems to me like we should use UpdateNVMeNamespace if we want to toggle enable/disable bit... instead of new RPC

we need to add enable/disable in NameSpaceSpec if we do it via namespace object; that'd work too, but having an admin know to enable a namespace on a subsystem/controller is the ask here.

@glimchb

glimchb commented Jan 26, 2023

Copy link
Copy Markdown
Member

we need to add enable/disable in NameSpaceSpec if we do it via namespace object; that'd work too, but having an admin know to enable a namespace on a subsystem/controller is the ask here.

agree, let's add Enable/Disable to NameSpaceSpec and then using Update RPC we can toggle it

@glimchb

glimchb commented Mar 17, 2023

Copy link
Copy Markdown
Member

@VaibhavJMarvell @jainvipin can you please continue this ? it is open for a long time

@glimchb glimchb linked an issue Sep 12, 2023 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Storage APIs or code related to storage area

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[storage]: Front-end Attach / Detach Namespace Proto-Bufs

3 participants